Skip to content

Implement a per interpreter language server cache - #8815

Merged
Rich Chiodo (rchiodo) merged 28 commits into
masterfrom
rchiodo/ls_per_kernel_2
Dec 2, 2019
Merged

Implement a per interpreter language server cache#8815
Rich Chiodo (rchiodo) merged 28 commits into
masterfrom
rchiodo/ls_per_kernel_2

Conversation

@rchiodo

Copy link
Copy Markdown

For #8206

Change how language servers are started and shutdown. Instead of there being only one at a time, allow many based on the interpreter in use.

The crux of this change is in the activationService.ts, It is now also a cache (it kinda had one already) that can be queried to return a language server.

Each language server

  • Registers as VS code's language server (the same way this used to work)
  • Allows an internal caller to also talk directly to the server (the new ILanguageServer API)

The reason for this change is because Notebooks can select a different python to use (called a kernel) than what is currently selected in the python selection.

Comment thread src/client/activation/languageServer/activator.ts Outdated
Comment thread src/client/activation/languageServer/activator.ts
Comment thread src/client/activation/languageServer/analysisOptions.ts
@codecov-io

Codecov (codecov-io) commented Nov 27, 2019

Copy link
Copy Markdown

Codecov Report

Merging #8815 into master will decrease coverage by 1.05%.
The diff coverage is 34.52%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8815      +/-   ##
==========================================
- Coverage   58.65%   57.59%   -1.06%     
==========================================
  Files         526      527       +1     
  Lines       27204    27124      -80     
  Branches     4059     4123      +64     
==========================================
- Hits        15956    15623     -333     
- Misses      10369    10617     +248     
- Partials      879      884       +5
Impacted Files Coverage Δ
src/client/providers/objectDefinitionProvider.ts 11.76% <ø> (-0.97%) ⬇️
src/client/datascience/types.ts 100% <ø> (ø) ⬆️
.../datascience/interactive-common/interactiveBase.ts 25.49% <ø> (+0.05%) ⬆️
src/client/ioc/types.ts 100% <ø> (ø) ⬆️
...science/jupyter/liveshare/guestJupyterExecution.ts 18.96% <0%> (ø) ⬆️
src/client/ioc/serviceManager.ts 35.48% <0%> (ø) ⬆️
src/client/providers/jediProxy.ts 23.37% <0%> (+0.17%) ⬆️
...active-common/intellisense/intellisenseDocument.ts 66.43% <0%> (-3.74%) ⬇️
src/client/datascience/serviceRegistry.ts 0% <0%> (ø) ⬆️
...t/activation/languageServer/languageServerProxy.ts 92.3% <100%> (ø)
... and 61 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 817199b...03c9869. Read the comment docs.

Comment thread src/client/activation/languageServer/languageClientMiddleware.ts Outdated
@rchiodo

Rich Chiodo (rchiodo) commented Nov 27, 2019

Copy link
Copy Markdown
Author

Actually I had another thought on my way home last night. I want to change the disconnect/reconnect pattern to activate/deactivate. Otherwise when we have a notebook switch kernels, we might end up activating a language server for VS code and have two activated at the same time (right now activate is called when the language server is created). I'm going to change activate to basically be reconnect, and have a 'start' on each language server which prepares it to be connected to VS code. Activate will then actually allow that language server to be the VS code language server. #Resolved

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still got a lot to review...

Comment thread src/client/activation/activationService.ts
Comment thread src/client/activation/activationService.ts
import { noop } from '../common/utils/misc';
import { ILanguageServer, LanguageServerActivator } from './types';

export class RefCountedLanguageServer implements ILanguageServer {

@DonJayamanne Don Jayamanne (DonJayamanne) Nov 27, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use Proxy class instead. Avoids the need to implement each method. #WontFix

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I thought there was some way to not have to implement each. Thanks will change.


In reply to: 351403997 [](ancestors = 351403997)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proxy class is messing up tests. Don and I talked and this seemed like the least exposed way to implement the ref counting idea.


In reply to: 351423686 [](ancestors = 351423686,351403997)

Comment thread src/client/activation/activationService.ts Outdated
Comment thread src/client/activation/activationService.ts
Comment thread src/client/datascience/jupyter/jupyterExecution.ts Outdated
Comment thread src/client/datascience/jupyter/jupyterCommandFinder.ts Outdated
Comment thread src/client/activation/languageServer/languageClientFactory.ts
Comment thread src/test/datascience/mockLanguageServerProxy.ts

@IanMatthewHuff Ian Huff (IanMatthewHuff) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@rchiodo

Copy link
Copy Markdown
Author

Don Jayamanne (@DonJayamanne) did you have any more changes?

@rchiodo
Rich Chiodo (rchiodo) merged commit 751f5b2 into master Dec 2, 2019
@rchiodo
Rich Chiodo (rchiodo) deleted the rchiodo/ls_per_kernel_2 branch December 2, 2019 20:20
@lock lock Bot locked as resolved and limited conversation to collaborators Dec 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants